fix(core): namespace SVG ids during composition inline to prevent cross-scene collisions - #3494
fix(core): namespace SVG ids during composition inline to prevent cross-scene collisions#3494miga-heygen wants to merge 2 commits into
Conversation
…ss-scene collisions Two nested compositions that each declare their own SVG ids (`<clipPath id="clip">`, `<symbol id="shape">`, `<filter id="fx">`) are legal per file and pass `hyperframes check`, but collide once both are inlined into one render/preview document. `url(#id)` funcrefs (`clip-path`, `filter`, `mask`, `fill`, `stroke`, `marker-start/mid/end`) and fragment `href`/`xlink:href` refs (`<use href="#id">`) are resolved by the browser's native SVG/CSS engine, which always binds to the first matching id in document order — so the later scene either clips to nothing or paints the earlier scene's content. `getElementById` was already scoped per composition in #646, and media pipeline ids got a parallel `data-hf-render-id` attribute in #3340. Neither covers this: native `url(#id)`/`href="#id"` resolution can't be intercepted by a JS proxy, so the `id` attribute itself has to become document-unique. Add `svgIdNamespacing.ts`: during `inlineSubCompositions`, every id declared on an `<svg>`-subtree element is prefixed with the composition's document-unique runtime id, and every same-document reference to it — DOM attributes (`href`, `xlink:href`, any `url(#id)` value including inside `style`) and the composition's own extracted `<style>` text (both `#id` selectors and `url(#id)` declaration values) — is rewritten to match. Renamed elements keep their original id on `data-hf-authored-id`, the same attribute #646 already added for the composition root, so an inline script's own `document.getElementById(originalId)` keeps resolving via the existing `__hfGetElementById` scoping shim. Extract `selectorIdTokens.ts` from `compositionScoping.ts`'s existing single-id selector scan so the new many-id rewrite reuses the same guarded-region logic instead of a second copy. Closes #3490 Co-Authored-By: Miga <noreply@anthropic.com>
vanceingalls
left a comment
There was a problem hiding this comment.
R1 adversarial — SVG ID collision namespace pass
Summary: DOM walk over svg[id]/svg [id] renames each declared id to ${sanitizedNamespace}--${originalId}, records the authored id on data-hf-authored-id, and rewrites same-element attribute refs via a url(#...) regex + suffix :href detector. <style> text goes through a postcss pass + a shared selectorIdTokens scan (extracted from the existing root-id rewrite so both callers share one quote/bracket state machine).
Blockers (P0/P1):
- (none)
Concerns (non-blocking):
- ARIA id-refs not rewritten —
aria-labelledby,aria-describedby,aria-controls,aria-owns,aria-flowtoall carry bare id lists (no#), and<svg><title id=\"chart-title\">referenced byaria-labelledby=\"chart-title\"is standard SVG a11y. Post-rename the id isscene-a--chart-title; the aria attr still sayschart-titleand either resolves to another scene's title in the merged doc or dangles. Not a first-order render bug (matches the ticket'surl(#)/hrefscope) but worth a followup — same class of native-resolver reference that motivated this PR. [id=\"foo\"]attribute selectors in<style>are silently skipped —selectorIdTokens.ts:markUnguardedOffsetsdeliberately masks out bracket regions (correct for#disambiguation), sorewriteSvgIdReferencesInCssnever touches[id=\"clip\"]. Rare in author CSS, but any composition that reaches for the attribute form instead of#clipwill silently stop matching after rename. Worth a test asserting current behavior + a doc note.- Idempotency not guarded — a second
namespaceSvgIds(root, ns)call double-prefixes (scene-a--scene-a--clip) AND overwritesdata-hf-authored-idwith the already-namespaced id, destroying the original.inlineSubCompositionslooks single-pass per instance so this is latent, but noif (el.hasAttribute(SVG_AUTHORED_ID_ATTR)) skipguard exists to catch a future re-entry (e.g. nested inline of a fragment that was itself pre-inlined). - No golden-frame / render-pixel test — three unit tests confirm the id map is correct, but nothing asserts "two scenes reusing
#fxactually paint their own filter after the compiler ran end-to-end." The repro in #3490 is visual; the failure mode is silent misresolution. A single regression-shard scene mirroring the ticket repro would lock the fix in against future refactors of the URL regex. sanitizeNamespaceSegmentfolds special chars to-without a collision guard —foo!andfoo?both becomefoo-, so any two runtime ids that differ only in special chars produce the same prefix. Runtime ids fromassignBundledRuntimeCompositionIdsare compiler-generated and almost certainly alphanumeric, but the sanitizer is a public-ish contract; a defensive test asserting the runtime-id shape would prevent surprise later.
Verified clean:
- URL fragment coverage:
url(#id),url(\"#id\"),url('#id'),url( #id ), and all funcref attrs (clip-path,filter,mask,fill,stroke,marker-start/mid/end) reach the same regex via the generic attribute walk — no per-attr whitelist to drift. - Fragment-
href:isHrefAttrNamecatcheshrefand everyfoo:hrefsuffix (coversxlink:hrefregardless of DOM impl), andrewriteHrefValueshort-circuits on non-#values sohttps://example.com/#clipand asset URLs stay untouched (explicit test). - Longer-id-shadowing:
selectorIdTokens.tssorts candidates longest-first and gates onisSelectorNameCharboundary, so#clipin the id map never eats#clip2. Explicit test in both suites. - No-SVG fast path:
querySelectorAll(\"svg [id], svg[id]\")returns empty →idMap.size === 0→ early return, no attribute walk. - Anonymous-host guard: empty
namespace→ early return with empty map, no mutation. Matches the same guardscopeCssToCompositionandwrapScopedCompositionScriptapply — consistent with existing scoping-primitive contract. getElementById(originalId)for author scripts:data-hf-authored-idis set on every renamed element;__hfGetElementByIdshim (from #646) already checks this attr as fallback. Explicit test assertsscene-aroot still resolvessymbolby authored idshapeafter rename.SVG_AUTHORED_ID_ATTRshares the exact string constant (\"data-hf-authored-id\") withAUTHORED_ROOT_ID_ATTRincompositionScoping.ts— the shim's fallback path already reads it, so no third attr introduced.- Shared scanner extraction:
replaceAuthoredRootIdSelectorsis now a thin wrapper overreplaceSelectorIdTokens; behavior preserved (single-form → one-element candidate list), one state machine to maintain instead of two. - Perf: single tree walk + O(N) attr scan + one postcss parse per composition — no visible O(N²).
CI: Preflight/lint/format/typecheck/unit/producer-integration/SDK/perf-drift/parity/fps/load/scrub/preview-parity/Fallow all green. regression-shards shards 1-9, Smoke: global install, Render/Tests on windows-latest, Test, Analyze (javascript-typescript) still pending — the render-parity signal is exactly what would exercise the visual repro so worth waiting on before merge.
Signature: — Via
| * character-by-character state machine. | ||
| */ | ||
| const GUARDED_SELECTOR_SEGMENT_RE = | ||
| /"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|\[(?:"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|[^\]])*\]/g; |
SVG ids referenced exclusively by JavaScript (e.g. GSAP's
`tl.to("#cut-1")`) must not be renamed — global libraries access
`document` directly and bypass the composition-scoped querySelector
Proxy, so renamed ids break animation targeting.
Now namespaceSvgIds pre-scans for url(#id) funcrefs and href="#id"
fragment refs (both attribute values and <style> text content) and
only renames ids that appear in at least one native reference. Ids
with no native reference (only used by JS) keep their original
names.
Fixes style-17-prod regression where GSAP selectors targeted
#cut-1/#tear-path-1 etc. which were unreachable after rename.
Co-Authored-By: miga-heygen <miguel.sierra_miga@heygen.com>
terencecho
left a comment
There was a problem hiding this comment.
COMMENT — R1 SVG id namespacing @ 75b84322586c06a4f8b9c84415a9e9a8465639a6. No P0/P1 blockers; three orthogonal follow-ups. Holding off APPROVE only because regression-shards (esp. shard-5 which contains style-17-prod) and preview-parity are still in-flight and are the load-bearing signals for both this fix AND the regression it patches.
Concurring with @vibook-bot's R1 (unchanged at head):
- ARIA id-refs (
aria-labelledby/-describedby/-controls/-owns/-flowto) still not tracked. Same class asurl(#)/href="#"— native browser resolution — worth a follow-up. [id="foo"]attribute-selector inside<style>intentionally skipped by the bracket-guard mask.- No idempotency guard on
SVG_AUTHORED_ID_ATTR— a re-entry double-prefixes and clobbers the original. - No golden-frame test locking the #3490 repro; leaning on regression-shards + preview-parity to catch it.
Independent concerns (differentiated):
-
Regression-fix trade-off worth documenting in-code. The
75b8432pivot to "only rename ids that have a nativeurl()/hrefreference" is the right call forstyle-17-prod(GSAP →document.querySelectorbypasses the scoped shim, so a renamed id is unreachable). But it introduces a residual cross-comp collision path: two composition instances that both animate the SAME JS-only id (e.g. two catalog scenes each doingtl.to("#cut-1")) now BOTH keepid="cut-1"→ merged doc has twoid="cut-1"→ scene B's GSAP targets scene A's element — the exact document-order-resolution bug this PR fixes, now residual for the JS-only subset. Sound trade-off (less common than the always-broken JS case), but the module doc should call it out so a future reader doesn't broaden the pre-scan and silently re-breakstyle-17. Suggest a comment nearcollectNativelyReferencedIdsnaming this residual class. -
Test gap paired with #1. No test asserts the two-comps-sharing-JS-only-id case — either as the "we accept this collision" contract or a scoped mitigation. Would lock in the current design.
-
Runtime-injected
url(#id)isn't pre-scanned. A<script>that later doesel.setAttribute("clip-path", "url(#foo)")as the ONLY reference to#foomisses the pre-scan →fooisn't renamed → same-class collision if duplicated across comps. Unlikely in HF composition authoring, but a known blind spot.
Verified clean at 75b8432:
url(#..)(bare/quoted/single-quoted/spaced), all funcref attrs (clip-path/filter/mask/fill/stroke/marker-*), inlinestyleattr, and<style>text content — all routed through the sameURL_HASH_REF_RE. No per-attr whitelist to drift.href/foo:href(coversxlink:hrefregardless of DOM impl); non-fragment hrefs (https://example.com/#clip) untouched, explicit test.- Pre-scan scope is per-comp (
namespaceSvgIds(innerRoot ?? contentDoc, ns)) — no cross-comp poisoning path. - Multi-instance same-src block:
assignBundledRuntimeCompositionIdsgives each instance a distinct runtime id → distinct namespace. End-to-end test locks it. data-hf-authored-idreuses the exact string constant fromcompositionScoping.ts—__hfGetElementByIdfallback already checks it, no third attr introduced.- Shared
selectorIdTokens.tsscanner:replaceAuthoredRootIdSelectorsis now a thin wrapper — one state machine to maintain, longest-first sort preserved (#clipnever eats#clip2). <style>extraction pipeline: sub-comp<style>textContent (including SVG-inline<style>) is extracted viaplan.styleSourcesand rewritten byrewriteSvgIdReferencesInCssbeforescopeCssToCompositionscopes it. Ordering is correct (rename → asset URL rewrite → composition scope).
CI: many required checks in-progress at snapshot; nothing failing. regression-shards (esp. shard-5 containing style-17-prod) and preview-parity are the load-bearing signals for both the primary fix AND the regression it patches — worth waiting on before merge. Happy to bump to APPROVE once those settle green.
— Review by tai (pr-review)
Summary
<clipPath id="clip">,<symbol id="shape">,<filter id="fx">, etc. no longer collide once two nested scenes are merged into one render/preview document.href,xlink:href, anyurl(#id)value —clip-path,filter,mask,fill,stroke,marker-start/mid/end, including insidestyle) and the composition's own extracted<style>text (both#idselectors andurl(#id)declaration values).data-hf-authored-id(the same attribute fix(core): scoped getElementById fails with duplicate element IDs across sub-compositions #646 added for the composition root), so an inline script's owndocument.getElementById(originalId)keeps resolving via the existing__hfGetElementByIdscoping shim — the fix doesn't regress the already-fixed getElementById scoping.selectorIdTokens.tsfromcompositionScoping.ts's existing single-id selector scan so the new many-id rewrite reuses the same guarded-region (quote/bracket) logic instead of a second copy.Why not the media-id / getElementById approach?
getElementByIdwas already scoped per composition in #646, and media pipeline ids got a paralleldata-hf-render-idattribute in #3340 — both work by adding a side-channel attribute without touching the realid. That doesn't work here:url(#id)andhref="#id"are resolved by the browser's native SVG/CSS engine, which always binds to the first element in document order carrying that literalidattribute. No JS proxy can intercept native resolution, so theidattribute itself has to become document-unique.Closes #3490
Test plan
packages/core/src/compiler/svgIdNamespacing.test.ts(new, 13 tests): unit coverage for id renaming,url()/hrefrewriting acrossclip-path/filter/mask/fill/stroke/marker-*/style,xlink:href, and CSS selector/declaration rewriting.packages/core/src/compiler/inlineSubCompositions.test.ts(new suite): end-to-end repro of the issue — two sibling scenes reusing#clip/#shape/#fxget distinct non-colliding ids that still resolve correctly; the same catalog block used twice in one scene is disambiguated;getElementById(originalId)still resolves for an inline script after rename.bun run typecheck— clean.oxlint— clean.compositionScoping.test.ts(50),htmlBundler.test.ts(58, exercisesinlineSubCompositionsend-to-end via the preview bundler) — no regressions.Co-Authored-By: Miga noreply@anthropic.com